feat(daemon): add POST /session/:id/language for runtime language switching#4705
feat(daemon): add POST /session/:id/language for runtime language switching#4705chiga0 wants to merge 8 commits into
Conversation
…tching Add a dedicated HTTP endpoint for switching UI language and LLM output language without polluting the session transcript. The endpoint flows through three layers (server route → bridge → ACP extMethod handler) following the same pattern as approval-mode and model switching. When syncOutputLanguage is true, the handler updates output-language.md, persists settings, and refreshes system prompts across all active sessions so the next LLM call immediately uses the new language. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
requested review from
doudouOUC and
wenshao
and removed request for
wenshao
📋 Review SummaryThis PR implements a runtime language-switching API endpoint ( 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Review Overview (AI Generated)PR: #4705 feat(daemon): add POST /session/:id/language for runtime language switching Multi-Round Review (Rounds 0-6): Clean — 0 findingsRound 0 (Design): Correct approach — three-layer flow (HTTP route → bridge extMethod → ACP handler) follows the established pattern used by Round 1 (Architecture): Clean implementation across all 5 files. Round 2 (Robustness):
Round 3 (Security): Round 4 (Performance): Round 5 (New Feature): Implementation matches PR description exactly — three-layer flow, language switching, optional output language sync, settings persistence, all-session refresh. Clean and focused, no unrelated changes. Round 6 (Undirected): Cross-file consistency verified — LGTM! This review was generated by QoderWork AI |
… debug logging - Replace hardcoded LANGUAGE_CODES array in server.ts with dynamically derived list from SUPPORTED_LANGUAGES, ensuring new languages added to the i18n module are automatically accepted by the API. - Add debugLogger.warn calls for settings persistence failures in the ACP handler instead of silently swallowing errors. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
Pull request overview
Adds a new POST /session/:id/language HTTP endpoint that switches the daemon's UI language and (optionally) the LLM output language at runtime, wired through the standard server → bridge → ACP extMethod three-layer pattern. When syncOutputLanguage is true, the handler also rewrites ~/.qwen/output-language.md, persists the user setting, and refreshes every active session's system prompt.
Changes:
- New ext-method
qwen/control/session/language,HttpAcpBridge.setSessionLanguage, andlanguage_changedbus event. - New Express route
POST /session/:id/languagewith allow-list validation againstSUPPORTED_LANGUAGES + 'auto'. QwenAgenthandler inacpAgent.tsperforms the global UI language change, settings persistence, and per-session system-prompt refresh.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/acp-bridge/src/status.ts | Registers the new sessionLanguage ext-method constant. |
| packages/acp-bridge/src/bridgeTypes.ts | Declares the setSessionLanguage interface on HttpAcpBridge. |
| packages/acp-bridge/src/bridge.ts | Implements the bridge method: forwards via extMethod, publishes language_changed. |
| packages/cli/src/acp-integration/acpAgent.ts | Adds the ACP-side handler that mutates global UI language, persists settings, and refreshes all sessions. |
| packages/cli/src/serve/server.ts | Adds the HTTP route, language-code validation, and bridge call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Overall
The three-layer pattern (server route → bridge → ACP extMethod) is well-followed, and deriving LANGUAGE_CODES dynamically from SUPPORTED_LANGUAGES (commit 2) is a good improvement over hardcoding. However, two critical issues need addressing before merge.
Additional items not covered by inline comments:
- No test coverage — 192 lines of production code, 0 lines of tests.
bridge.test.tshas tests forsetSessionApprovalMode(lines ~5083-5306);setSessionLanguageshould have comparable coverage: basic call path, session-not-found error, event publish verification, and concurrent-request serialization (if queue is added). language_changedevent has no consumer — the event type only appears in this PR's new code. No SSE consumer, no frontend handler, no test references it. If the consumer comes in a follow-up PR, please note that in the description.- Response lacks
sessionId—setSessionApprovalModereturns{ sessionId, mode, previous, persisted }.setSessionLanguagereturns{ language, outputLanguage, refreshed }withoutsessionId. Minor inconsistency but worth aligning for API uniformity.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] No tests for the new POST /session/:id/language endpoint. +192 lines of new logic across 3 layers (server route, bridge forwarding, ACP handler) with zero test coverage. Existing server.test.ts has analogous test suites for model and approval-mode endpoints covering success, validation errors, 404, client identity forwarding, and bridge error propagation. Critical branches uncovered: valid/invalid language, non-boolean syncOutputLanguage, missing session (404), client-id forwarding, and the syncOutputLanguage=true path that mutates settings files and refreshes all sessions.
[Suggestion] Missing session_language capability registry entry in capabilities.ts. Every other session-level mutation endpoint has one (session_set_model, session_approval_mode_control, session_recap, session_btw). SDK clients preflighting via GET /capabilities cannot discover language-switching support.
— qwen3.7-max via Qwen Code /review
- Add sessionOrThrow() call for session existence validation (doudouOUC) - Wrap setLanguageAsync in try-catch with structured error (doudouOUC) - Wrap updateOutputLanguageFile in try-catch to prevent partial state (wenshao) - Return resolved language code instead of echoing "auto" verbatim (wenshao) - Add refreshed field to language_changed SSE event payload (wenshao) - Add language to telemetry route regex (wenshao) - Add FakeBridge setSessionLanguage and 6 server route tests Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review Response — af375f4Thanks for the thorough reviews. Here's how each finding was handled: Fixed (in commit af375f4)
Fixed earlier (in commit d5e5a9d)
Won't fix (with reasoning)Concurrency serialization queue (doudouOUC) — Language switching is idempotent: the last writer wins and the result is consistent regardless of ordering. This differs fundamentally from approval-mode (non-idempotent state machine transitions where order determines the final state). The existing `/language` slash command has no serialization either. Workspace broadcast for peer sessions (doudouOUC) — The ACP handler already refreshes all sessions' system prompts via `Promise.allSettled`. The SSE event is for UI state updates; in practice, only one frontend client is attached to a session. Adding `broadcastWorkspaceEvent` would send duplicate events to sessions whose prompts are already refreshed. Missing `previousLanguage` in return type (doudouOUC) — The caller already knows the current language (it's what's displayed in their UI). Unlike approval-mode where the previous state matters for audit trails, language switching is a simple preference toggle. `refreshed: true` even when individual sessions failed (doudouOUC/Copilot) — `refreshed` reflects whether the refresh phase was attempted, not whether every individual session succeeded. The core mutation (file write + settings persist) is what matters; individual session refresh failures are transient and self-correcting on next prompt. Unbounded `Promise.allSettled` concurrency (wenshao) — Typical daemon has 1-5 sessions. The `initTimeoutMs` (60s) provides ample headroom. Adding a concurrency cap for a single-digit session count would be over-engineering. `withTimeout` too tight (wenshao) — `initTimeoutMs` defaults to 60s, same as all other extMethods. `setLanguageAsync` loads a small JSON file (~1ms), `writeFileSync` writes a small markdown file (~1ms), and `Promise.allSettled` refreshes 1-5 sessions (~100ms each). Total < 1s in practice. `language_changed` SSE event has no consumer (doudouOUC) — The consumer is in the frontend codebase (agent-web), not in qwen-code. This follows the same pattern as `approval_mode_changed`, `model_switched`, `tool_toggled` — all published here, consumed by frontend SSE subscribers. SDK event type registration (Copilot) — Valid but out of scope for this PR. Can be added when the SDK is updated to consume this event. Route scope: session vs workspace (Copilot) — `setLanguageAsync` is process-level, but the route needs `sessionId` for ACP channel routing and `clientId` authentication. This matches the `model` switching pattern (also process-level state routed through session endpoints). ACP handler language validation (Copilot) — The HTTP route already validates against `LANGUAGE_CODES`. The ACP handler is only reachable through the bridge, which always goes through the route first. Defense-in-depth can be added later. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
(Posted as a body-level comment because the tsc error is on a line not touched by this PR's diff.)
[Nice to have] server.test.ts:752 — tsc reports FakeBridge is missing executeShellCommand from HttpAcpBridge. This is pre-existing from the base branch (not caused by this PR's changes), but will cause npm run typecheck to fail. Add a stub to FakeBridge:
async executeShellCommand() {
throw new Error('not implemented');
},— qwen3.7-max via Qwen Code /review
When language is "auto", persist the literal "auto" to settings instead of the resolved concrete locale. This ensures auto-detection via detectSystemLanguage() is re-evaluated on daemon restart rather than being permanently pinned to whatever locale was resolved at switch time. The response still returns the resolved language via getCurrentLanguage(). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Mirror the LANGUAGE_CODES allowlist from the HTTP route into the ACP extMethod handler, so direct extMethod callers are also validated. Follows the same pattern as the approval-mode handler. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Only set refreshed=true when at least one session refresh succeeded. Log the count of failed sessions for diagnostics. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
PR Verification ReportPR: #4705 — feat(daemon): add POST /session/:id/language for runtime language switching Test Results
Pre-existing Verification
Code ReviewChanges (6 files, +357/−1):
Key observations:
Verdict✅ Ready to merge — Clean implementation following existing daemon API patterns. All 5 PR-specific functional tests pass; the 1 remaining failure and all 38 pre-existing failures share the same Verified by wenshao |
…ponse Add ?? null guard to outputLanguage in the language_changed SSE event payload, matching the HTTP response path. Without this, an undefined value would be silently omitted by JSON.stringify instead of being explicitly null. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
| try { | ||
| updateOutputLanguageFile(settingValue); | ||
| } catch (err) { | ||
| debugLogger.warn('Failed to write output-language.md:', err); |
There was a problem hiding this comment.
[Suggestion] When updateOutputLanguageFile throws (caught above), the handler continues to refreshHierarchicalMemory() on all sessions — which re-reads the output-language.md file from disk. Since the write failed, the file still contains the old language rule, so every session's system prompt is rebuilt with stale content. Yet the handler returns outputLanguage: resolved (the intended new value) and may report refreshed: true, misleading the caller into thinking the switch took effect.
Consider tracking whether the file write succeeded and either skipping the refresh or signaling failure:
| debugLogger.warn('Failed to write output-language.md:', err); | |
| let fileWriteOk = false; | |
| try { | |
| updateOutputLanguageFile(settingValue); | |
| fileWriteOk = true; | |
| } catch (err) { | |
| debugLogger.warn('Failed to write output-language.md:', err); | |
| } |
Then guard the refresh loop and response on fileWriteOk.
— qwen3.7-max via Qwen Code /review
| debugLogger.warn( | ||
| `Language refresh failed for ${failedCount}/${results.length} session(s)`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] refreshed = failedCount < results.length has two edge-case issues:
- Partial success: if 1 of 5 sessions fails,
refreshed = true. The caller cannot distinguish full from partial refresh. - Zero sessions: when
this.sessionsis empty,0 < 0 === false. Nothing failed — there was simply nothing to refresh — yet the response saysrefreshed: false.
Consider refreshed = failedCount === 0 (zero failures = refreshed) or returning structured counts (refreshedSessions, failedSessions) so callers can make their own judgment.
— qwen3.7-max via Qwen Code /review
| const bridge = fakeBridge(); | ||
| const app = createServeApp(baseOpts, undefined, { bridge }); | ||
| const res = await request(app) | ||
| .post('/session/session-A/language') |
There was a problem hiding this comment.
[Suggestion] This test asserts res.status and the bridge call params, but not res.body. If the route handler accidentally returned a wrong shape when syncOutputLanguage is omitted (e.g., always returning refreshed: true), this test would still pass. Adding expect(res.body).toEqual({ language: 'en', outputLanguage: null, refreshed: false }) would verify the user-visible response.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('POST /session/:id/language', () => { |
There was a problem hiding this comment.
[Suggestion] The six tests cover success, validation errors, and SessionNotFoundError (404), but none exercise the generic 500 path — i.e., when the bridge throws a non-SessionNotFoundError Error. The sendBridgeError fallback to 500 is uncovered for this endpoint. Adding a test where setLanguageImpl throws new Error('boom') would cover the full error surface.
— qwen3.7-max via Qwen Code /review
|
|
||
| const LANGUAGE_CODES = [...SUPPORTED_LANGUAGES.map((l) => l.code), 'auto']; | ||
|
|
||
| app.post('/session/:id/language', mutate(), async (req, res) => { |
There was a problem hiding this comment.
[Suggestion] The new POST /session/:id/language endpoint is not registered in the daemon capabilities registry (capabilities.ts). Every other session control endpoint has a corresponding capability tag (e.g., session_set_model, session_approval_mode_control). SDK clients use GET /capabilities for feature detection — without a tag, they cannot programmatically discover runtime language switching support.
— qwen3.7-max via Qwen Code /review
Summary
POST /session/:id/languageHTTP endpoint for switching UI language and LLM output language at runtime without polluting session transcriptsetSessionLanguage()→ ACP extMethod handler, following the same pattern asapproval-modeandmodelswitchingsyncOutputLanguage: true, updateoutput-language.md, persist settings, and refresh system prompts across all active sessions so the next LLM call immediately uses the new languageChanges
packages/acp-bridge/src/status.tssessionLanguagetoSERVE_CONTROL_EXT_METHODSpackages/acp-bridge/src/bridgeTypes.tssetSessionLanguage()toHttpAcpBridgeinterfacepackages/acp-bridge/src/bridge.tssetSessionLanguage()bridge methodpackages/cli/src/acp-integration/acpAgent.tssessionLanguageextMethod handlerpackages/cli/src/serve/server.tsPOST /session/:id/languagerouteAPI
Response (200):
{ "language": "zh", "outputLanguage": "Chinese", "refreshed": true }Supported language codes:
zh,zh-TW,en,ja,ru,de,fr,pt,ca,autoTest plan
POST /session/:id/languagewith{"language":"en","syncOutputLanguage":true}→ verify 200 response withoutputLanguage: "English"invalid_languagelanguagefield → 400syncOutputLanguageomitted → 200 withoutputLanguage: null~/.qwen/output-language.mdcontent matches the switched language🤖 Generated with Qwen Code